-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
feat: Add displaying multiple info panels and batch navigation #3020
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
🚀 Thanks for opening this pull request! We appreciate your effort in improving the project. Please let us know once your pull request is ready for review. |
📝 WalkthroughWalkthroughAdds multi-panel support to the data browser: panel management UI (Add/Remove panels, Show/Hide), synchronized scrolling and wheel handling, batch navigation with adjusted prefetch semantics, per-panel data state/fetching, localStorage persistence for panel settings, and supporting UI/CSS updates. README prefetch wording clarified. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant Toolbar
participant BrowserToolbar
participant DataBrowser
participant MultiPanelUI
User->>Toolbar: Click "Add Panel"
Toolbar->>BrowserToolbar: (prop) addPanel()
BrowserToolbar->>DataBrowser: addPanel()
DataBrowser->>DataBrowser: panelCount++\ninitialize multiPanelData entry\npersist settings
DataBrowser->>MultiPanelUI: render additional panel column
User->>MultiPanelUI: Scroll a panel
MultiPanelUI->>DataBrowser: handlePanelScroll(event, index)
alt syncPanelScroll enabled
DataBrowser->>MultiPanelUI: update scroll positions for all panels
end
User->>MultiPanelUI: Wheel scroll (batch mode)
MultiPanelUI->>DataBrowser: handleWrapperWheel(event)
alt batchNavigate enabled
DataBrowser->>DataBrowser: apply delta across panels\nfetchDataForMultiPanel() -> trigger prefetch
Note right of DataBrowser: Prefetch count = prefetchObjects × panelCount
end
User->>Toolbar: Click "Remove Panel"
Toolbar->>BrowserToolbar: (prop) removePanel()
BrowserToolbar->>DataBrowser: removePanel()
DataBrowser->>DataBrowser: panelCount--\ncleanup multiPanelData entry
DataBrowser->>MultiPanelUI: re-render panels
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
src/dashboard/Data/Browser/Databrowser.scss (1)
27-90: Consider keyboard/a11y for panel header checkbox
panelHeaderis clickable while the nestedinput[type="checkbox"]ispointer-events: none, and the headerdivitself is not focusable. That means keyboard users can’t toggle selection easily, and screen readers may expose a checkbox that doesn’t respond to direct interaction. Consider either:
- Making the checkbox itself the interactive control (no
pointer-events: none, with properonChangewiring), or- Adding
tabindex={0}and keyboard handlers (Enter/Space) to the header while treating the checkbox as purely decorative (aria-hidden="true").This would keep the current UX while improving accessibility.
src/components/Toolbar/Toolbar.react.js (1)
161-188: Add PropTypes for new panel management propsThe Add/Remove/Show/Hide panel controls are wired correctly and gated on
classwiseCloudFunctions, but the new props (addPanel,removePanel,panelCount,togglePanel,isPanelVisible) aren’t reflected inToolbar.propTypes. Adding them (with the appropriate function/number/bool types) would make misuse easier to catch.src/dashboard/Data/Browser/DataBrowser.react.js (4)
80-136: Multi-panel state, persistence, and selection handling – behavior check and cleanupA few points around the new state/persistence wiring:
New localStorage-backed flags default to “on”
storedSyncPanelScroll,storedBatchNavigate, andstoredShowPanelCheckboxare initialized withgetItem(...) !== 'false', so the features are enabled by default even when no key exists yet. Toggling stores'true'/'false'.- This effectively turns these features on for existing users after upgrade. If that’s intentional, this is fine; if you’d prefer opt-in behavior, consider switching the reads to
=== 'true'instead.setSelectedObjectId multi-panel batch logic
- The batch calculation around
displayedObjectIdsand_objectsToFetchlooks sound (selected row anchored at start of batch, pulling up topanelCountobjects, using cache when fresh).- Near the end of the dataset,
panelCountcan exceeddisplayedObjectIds.length(e.g. adding a panel when there aren’t enough remaining rows). That’s harmless but meansbatchNavigateand prefetch will step bypanelCounteven though fewer panels are actually rendered. If you want UI and behavior to stay in lockstep, you could clamp the effective step size todisplayedObjectIds.length.Minor style cleanup from static analysis
- Inside
setSelectedObjectId,newMultiPanelDatais never reassigned, only mutated, so it can be aconst:
let newMultiPanelData = { ...prevState.multiPanelData };
const newMultiPanelData = { ...prevState.multiPanelData };Overall the selection and state wiring look correct; the above are mainly behavioral choices and small polish items.
Also applies to: 159-171, 776-848, 886-908
229-245: Scroll synchronization: avoid potential scroll churn and clarify scroll targetThe scroll-sync plumbing generally works, but there are a couple of edge cases to consider:
Possible scroll event ping-pong between panels
handlePanelScrollcopiesevent.target.scrollTopto all otherpanelColumnRefs. Those programmatic updates will in turn fireonScrollon the other columns, which then write back to the origin column. Most browsers suppress redundant events when the value is unchanged, but this isn’t guaranteed and can cause extra scroll events.- You can guard against this cheaply by only writing when the delta is non-zero, e.g.:
const scrollTop = event.target.scrollTop; this.panelColumnRefs.forEach((ref, i) => { if (i !== index && ref?.current && ref.current.scrollTop !== scrollTop) { ref.current.scrollTop = scrollTop; } });Nested scrolling vs. “Scroll to top” behavior
- The container
aggregationPanelRefstill hasoverflow: auto, while each.panelColumnalso hasoverflow-y: auto. In multi-panel mode, most of the vertical scrolling will happen inside columns, but the “Scroll to top” setting only resetsthis.aggregationPanelRef.current.scrollTop. Users can end up with columns scrolled independently even after an auto-reset.- If you want “Scroll to top” to fully reset multi-panel layout, consider also resetting each
panelColumnRef.current.scrollTopwhen that flag is enabled.The current implementation is functional; these changes would make scroll behavior more predictable and robust.
Also applies to: 297-324, 910-939
941-990: Reduce duplicate Cloud function calls between multi-panel fetch and prefetch
fetchDataForMultiPanelandprefetchObjectboth callParse.Cloud.runwith the samecloudCodeFunctionand object pointer, but via different paths:
setSelectedObjectIdcollects_objectsToFetchand callsfetchDataForMultiPanelfor each (current batch).handlePrefetchdetects a navigation pattern and callsprefetchObjectfor upcoming objects (future batches).Because
handlePrefetch()is invoked at the end ofsetSelectedObjectId’s callback, you can end up with both functions firing separate network requests for the same object (especially in multi-panel mode where upcoming batch rows overlap withdisplayedObjectIds).To avoid redundant Cloud calls and reduce load:
- Consider having
fetchDataForMultiPanel(objectId)delegate toprefetchObject(objectId)and only augmentmultiPanelDatafrom the existingprefetchCache, or- Extract a shared “fetch once and update both caches” helper that both
prefetchObjectandfetchDataForMultiPaneluse.That would ensure each object’s panel data is fetched at most once per staleness window while keeping the current caching and media-prefetch behavior.
Also applies to: 1095-1127, 1231-1257
1414-1486: Multi-panel rendering: loading UX and selection/checkbox accessibilityThe multi-panel rendering block is a nice composition, but there are a few UX/a11y details worth tightening:
Loading state for secondary panels
- Panels for
displayedObjectIdsother than the currently selected row derivepanelDatafrommultiPanelData[objectId] || {}and useisLoading = objectId === selectedObjectId && isLoadingCloudFunction.- When
fetchDataForMultiPanelis fetching data for non-selected objects, those panels render with emptydataandshowAggregatedData={true}, which leadsAggregationPanelto show “No object selected.” rather than a loading indicator.- If you want to signal that data is on the way, consider tracking a per-object loading flag (or checking presence in
_objectsToFetch) and passingisLoadingCloudFunctionaccordingly so secondary panels show a spinner instead of “No object selected.” while fetching.Panel header checkbox interaction
- The header
divhasonClicktogglingselectRow(objectId, !isRowSelected), while the nested<input type="checkbox">isreadOnlyand rendered purely as a visual indicator. Combined withpointer-events: nonein the SCSS, the checkbox isn’t directly interactive, and the headerdivisn’t keyboard-focusable.- For better accessibility, consider:
- Making the checkbox itself the interactive element (handling
onChangeand letting it focus), or- Treating the checkbox as decorative (
aria-hidden) and addingtabIndex={0}+ key handlers on the header so keyboard users can toggle selection.Scroll-to-top vs multi-panel columns
- As noted earlier, the “Scroll to top” behavior still only touches
aggregationPanelRef.scrollTop. If you decide that setting should also reset per-panel scrolls, this render block is the right place to apply those resets via thepanelColumnRefs.Behavior is otherwise coherent: the correct error state is only passed to the currently selected object, and separators are rendered cleanly between panels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
README.md(1 hunks)src/components/Toolbar/Toolbar.react.js(1 hunks)src/components/Toolbar/Toolbar.scss(2 hunks)src/dashboard/Data/Browser/BrowserToolbar.react.js(3 hunks)src/dashboard/Data/Browser/DataBrowser.react.js(14 hunks)src/dashboard/Data/Browser/Databrowser.scss(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2025-07-31T06:12:17.707Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2957
File: src/dashboard/Data/Browser/BrowserTable.react.js:584-597
Timestamp: 2025-07-31T06:12:17.707Z
Learning: In Parse Dashboard's data browser selection logic (src/dashboard/Data/Browser/BrowserTable.react.js), the `selection['*']` pattern is used to handle global operations that pass `{ '*': true }` to indicate all items are selected, particularly for bulk operations like delete. This is not dead code but serves as compatibility layer for global operations that don't go through normal individual row selection workflows.
Applied to files:
src/dashboard/Data/Browser/DataBrowser.react.js
📚 Learning: 2025-05-27T12:09:47.644Z
Learnt from: mtrezza
Repo: parse-community/parse-dashboard PR: 2828
File: src/dashboard/Data/Browser/Browser.react.js:1605-1607
Timestamp: 2025-05-27T12:09:47.644Z
Learning: In script execution dialogs in Parse Dashboard (specifically the `confirmExecuteScriptRows` method in `src/dashboard/Data/Browser/Browser.react.js`), individual `setState` calls to update `processedScripts` counter should be kept as-is rather than batched, because this provides real-time progress feedback to users in the dialog UI.
Applied to files:
src/dashboard/Data/Browser/DataBrowser.react.js
🧬 Code graph analysis (2)
src/dashboard/Data/Browser/BrowserToolbar.react.js (3)
src/components/BrowserMenu/MenuItem.react.js (1)
MenuItem(11-58)src/components/Icon/Icon.react.js (1)
Icon(12-29)src/parse-interface-guide/ComponentsMap.js (2)
Icon(40-40)Icon(40-40)
src/dashboard/Data/Browser/DataBrowser.react.js (1)
src/components/AggregationPanel/AggregationPanel.js (1)
AggregationPanel(15-223)
🪛 GitHub Check: Lint
src/dashboard/Data/Browser/DataBrowser.react.js
[failure] 791-791:
'newMultiPanelData' is never reassigned. Use 'const' instead
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Node 22
- GitHub Check: Node 20
- GitHub Check: Docker linux/amd64
🔇 Additional comments (3)
README.md (1)
1379-1385: Prefetching description aligns with multi-panel implementationThe updated
prefetchObjectswording (navigation steps andprefetchObjects × panelCountin multi-panel mode) matches the currenthandlePrefetchlogic inDataBrowser.react.js, so the docs look accurate.src/components/Toolbar/Toolbar.scss (1)
129-171: Panel button styling looks consistent and scopedThe new
.panelButtonscontainer and.btntweaks (padding, nowrap, disabled state) are cohesive with the toolbar layout and safely scoped via CSS modules; no issues spotted.src/dashboard/Data/Browser/BrowserToolbar.react.js (1)
81-99: Multi-panel controls and Info Panel settings wiring look correctThe new panel-management props (
addPanel,removePanel,panelCount) are correctly threaded fromBrowserToolbarintoToolbar, and the three Info Panel settings (sync scroll, batch navigation, panel selection) follow the existing checked-menu-item pattern. No behavioral issues stand out here.Also applies to: 284-300, 424-480
# [8.1.0-alpha.2](8.1.0-alpha.1...8.1.0-alpha.2) (2025-11-26) ### Features * Add displaying multiple info panels and batch navigation ([#3020](#3020)) ([7ac3186](7ac3186))
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/components/Toolbar/Toolbar.react.js (1)
161-189: Panel controls wiring looks solid; consider UX tweak for Add PanelThe visibility and enablement logic for Add/Remove/Show/Hide is consistent with
panelCountandisPanelVisible, and matches the DataBrowser API. Only minor UX nit:addPanelis callable even when no row is selected, in which case it silently does nothing. If that’s a common case, consider disabling Add Panel (or showing a tooltip) until a row is selected so the behavior is more obvious.src/dashboard/Data/Browser/DataBrowser.react.js (5)
789-848: Multi-panel batch selection and deferred fetch pipeline is well-structuredThe extended
setSelectedObjectIdlogic correctly:
- Tracks selection history for prefetch heuristics.
- Recomputes
displayedObjectIdsonly when a new object falls outside the current multi-panel batch.- Reuses cached data where available and stages missing objects for later async fetch via
_objectsToFetch.Using a functional
setStateplus a post-update callback avoids race conditions. If you want to reduce state churn later,_objectsToFetchcould be moved to an instance field, but that’s an optional micro-optimization.
886-908: Sync scroll and batch toggles behave correctly; scroll sync implementation is reasonableThe three toggle helpers correctly flip state and persist to
localStorage, aligning with the default-on pattern used at initialization. Scroll syncing (handlePanelScrollandhandleWrapperWheel) is straightforward and should work well in practice; any redundant scroll events will quickly converge without affecting correctness.Also applies to: 910-939
941-990: Per-object multi-panel fetch is correct; consider DRY’ing withprefetchObject
fetchDataForMultiPanelcorrectly honors prefetch staleness, reuses cached entries, and updates bothprefetchCacheandmultiPanelDataon fresh fetches. It duplicates some logic fromprefetchObject, so if this grows further it might be worth centralizing the Parse.Cloud.run + cache update logic in a shared helper.
992-1055: Panel add/remove logic is sound; only minor polish possible
addPanelcorrectly builds a contiguous batch starting at the selected row, hydratesmultiPanelDatafrom the live panel or cache, and defers missing fetches.removePanelcleanly decrementspanelCountand trimsdisplayedObjectIds.Minor notes (optional):
- When
currentIndexis-1(no valid selection),addPanelis a silent no-op; pairing this with a disabled Add Panel button or tooltip would make the UX clearer.- In
removePanel, returningnullinstead of{}in thepanelCount <= 1case would avoid an unnecessary re-render.
1414-1486: Multi-panel rendering path is coherent and matches the data modelThe multi-panel branch correctly:
- Uses
displayedObjectIdsandmultiPanelDatato drive per-panel content.- Applies per-column scroll refs and onScroll handlers for sync behavior.
- Integrates optional selection checkboxes and reuses the existing
AggregationPanelAPI.The single-panel fallback remains unchanged, so the feature is additive and backward-compatible. If in the future you need per-panel error reporting, you may want to model
errorAggregatedDatabyobjectId, but the current shared handling is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/components/Toolbar/Toolbar.react.js(1 hunks)src/dashboard/Data/Browser/DataBrowser.react.js(14 hunks)src/dashboard/Data/Browser/Databrowser.scss(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/dashboard/Data/Browser/Databrowser.scss
🧰 Additional context used
🧬 Code graph analysis (1)
src/dashboard/Data/Browser/DataBrowser.react.js (1)
src/components/AggregationPanel/AggregationPanel.js (1)
AggregationPanel(15-223)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Docker linux/amd64
🔇 Additional comments (6)
src/dashboard/Data/Browser/DataBrowser.react.js (6)
95-101: LocalStorage-backed defaults are consistent; default-on behavior is clearThe new sync scroll, batch navigation, and checkbox flags are correctly initialized from
localStorage, with sensible default-on semantics via!== 'false'. The additional state fordisplayedObjectIds,panelCount,multiPanelData, and_objectsToFetchis coherent and sets up the multi-panel model cleanly.Also applies to: 127-136
159-167: Method binding and new refs are consistent with existing patternsBinding the new toggle/add/remove/scroll handlers in the constructor and introducing
panelColumnRefs/multiPanelWrapperReffit the existing class style and enable the required scroll syncing without surprises.Also applies to: 169-171
635-637: Batch navigation step sizing for Up/Down looks correctThe use of
stepSize = panelCountwhenbatchNavigateis enabled (otherwise 1) is a clean way to move in batches. TheMath.max/Math.minclamping correctly prevents out-of-bounds indices, and Ctrl/Meta behavior remains intact.Also applies to: 679-685
1095-1127: Prefetch heuristic cleanly extended to multi-panel batchesThe updated prefetch logic correctly infers a navigation step size from recent selections and prefetches ahead accordingly. Extending it to also prefetch the rest of each upcoming batch when
panelCount > 1is a good touch and should keep multi-panel views responsive without over-fetching.
1241-1247: KeepingmultiPanelDatain sync with cache hits is correctMirroring cached panel data into
multiPanelDatainsidehandleCallCloudFunctionavoids redundant fetches when switching between single and multi-panel modes and ensures consistent data across both paths.
1519-1533: Toolbar wiring for multi-panel controls is consistentPassing
addPanel,removePanel,panelCount, and the three new toggle flags plus handlers intoBrowserToolbarcleanly exposes the new behavior to the UI without altering existing props. This lines up with the updatedToolbarcomponent.
|
🎉 This change has been released in version 8.1.0-alpha.2 |
New Pull Request Checklist
Issue Description
Currently, the info panel can only show 1 object at a time. That is inefficient when traversing many objects.
Approach
Add displaying multiple info panels in parallel and batch navigation to quickly traverse batches of objects.
Added new menu options:
TODOs before merging
Summary by CodeRabbit
New Features
Documentation
UI/Style
✏️ Tip: You can customize this high-level summary in your review settings.